-
Notifications
You must be signed in to change notification settings - Fork 60
Use reshape/ravel to set numpy shape #1165
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
For reference:
- Unlike
numpy.reshapethe instance methodndarray.reshapecan take either a tuple but also an unpacked tuple. This PR mixes both conventions. I don't mind. reshapeandravelonly copies the array when needed. In contrastflatten(which we do not use in this PR) always copies.
As for the cython generated code, I think we should do it there as well (not manually). But that can be another PR.
For example this will break in numpy 2.4.0 because to the strides trick:
} else {__pyx_pybuffernd_mask.diminfo[0].strides = __pyx_pybuffernd_mask.rcbuffer->pybuffer.strides[0]; __pyx_pybuffernd_mask.diminfo[0].shape = __pyx_pybuffernd_mask.rcbuffer->pybuffer.shape[0];
See for example silx-kit/pyFAI#2711.
These .c files say /* Generated by Cython 0.29.32 */. Not sure what's going on here. Why are they in the repo @vasole ?
Edit: it seems like setup.py anticipates cython not being available in which case it falls back to the c files. We installed cython and re-generated the .c files but no changes were applied, even when numpy 2.4.0 was installed.
| numpy.sum(tmpData, 2), | ||
| self._stackImageData[i:i+step,:]) | ||
| tmpData.shape = step*shape[1], shape[2] | ||
| tmpData = tmpData.reshape(step*shape[1], shape[2]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I did not know that elements of the shape parameter can be passed in as separate arguments.
https://numpy.org/devdocs/reference/generated/numpy.ndarray.reshape.html
FYI The free numpy.reshape function only accepts a tuple.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but you did it in the suggestion for issue #1162
New pattern using ndarray.reshape
arr = numpy.zeros((1,3,4));arr = arr.reshape(-1, 6);print(arr.shape)
(2, 6)
and to be concistent between
arr.reshape(X, Y) + arr.reshape(*tuple)
or
arr.reshape((X, Y)) + arr.reshape(tuple)
I chose 1st option
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed. I just didn't know before. I've only used the free numpy.reshape until I looked at the easiest way to refactor setting shape.
| else: | ||
| ddict['result'][label] = ymatrix | ||
| ddict['result'][label].shape = (len(ddict['result'][label]),) | ||
| ddict['result'][label] = numpy.ravel(ddict['result'][label]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's indeed another case where we can use numpy.ravel.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sergey-yaroslavtsev We introduced silent flattening here. See #1170 (comment).
| if not gotIt: | ||
| raise ValueError("Unmatched dimensions following C order") | ||
| data.shape = xsize, oldDataShape[i+1:] | ||
| data = data.reshape(xsize, *oldDataShape[i+1:]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. This was a bug.
| ncols = values[2] | ||
| self.nSpectra = nrows * ncols | ||
| data.shape = [len(data)/3, 3] | ||
| data = data.reshape(int(len(data)/3), 3) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch
| else: | ||
| spectra = data[badMask, iXMin:iXMax+1] | ||
| spectra.shape = badMask.sum(), -1 | ||
| spectra = spectra.reshape(int(badMask.sum()), -1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch.
Co-authored-by: Wout De Nolf <[email protected]>
|
Thank you! |
woutdenolf
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I propose the fix the three silent flattenings we introduced here. See #1170 (comment) for the preferred way.
| else: | ||
| ddict['result'][label] = ymatrix | ||
| ddict['result'][label].shape = (len(ddict['result'][label]),) | ||
| ddict['result'][label] = numpy.ravel(ddict['result'][label]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sergey-yaroslavtsev We introduced silent flattening here. See #1170 (comment).
| def diago(self, k, m): | ||
| mat = numpy.zeros([m,m], numpy.float64) | ||
| mat.shape=[m*m] | ||
| mat = numpy.ravel(mat) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sergey-yaroslavtsev We introduced silent flattening here. See #1170 (comment).
| xdata.shape= [len(xdata),] | ||
| ydata.shape= [len(ydata),] | ||
| xdata = numpy.ravel(xdata) | ||
| ydata = numpy.ravel(ydata) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sergey-yaroslavtsev We introduced silent flattening here. See #1170 (comment).
Closes #1162
Following suggestions from comments in the issue to switch all
.shap=to eitherreshapeorravel.Fixed places are less than mentioned "485". This ".shape=" remained in
.cfiles and in some comments, where for my understanding they should not be changed. Or am I wrong?In
.pyxfiles it was changed.runs OK.
Please have a brief look.
Note:
in many cases I used "*" to unpack the input shape but it is not required - now I doubt if it makes things clearer or worse.